Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bluetooth: Define connection event prepare callbacks #16099

Merged
merged 2 commits into from
Jul 17, 2024

Conversation

rugeGerritsen
Copy link
Contributor

The connection event callback feature replaces
the radio notification feature. It can be used to
synchronize data sampling with Bluetooth connection events.

The old feature had several weaknesses:

  • It was not possible to know which connection
    the notification belonged to.
  • The trigger also happened upon flash access.
  • The time from trigger until event start was fixed.
    That is, not configurable per use case.

The new feature is built on top of the connection event
trigger feature and is therefore able to known
which connection a given notification belongs to.

This PR is done on top of #15798 to simplify the implementation

@github-actions github-actions bot added doc-required PR must not be merged without tech writer approval. changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Jun 20, 2024
@rugeGerritsen rugeGerritsen changed the title Bluetooth: Define connection event nofitication callbacks Bluetooth: Define connection event prepare callbacks Jun 20, 2024
@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

@rugeGerritsen
Copy link
Contributor Author

Other suggestion: Add the possibility to trigger a callback at the start of the events.

Then it might make sense to define the callback as a struct so that it is possible to later add start() and end() if requested.

struct bt_conn_evt_cb {
	 void (*prepare)(struct bt_conn *conn, void *user_data);
	 void (*start)((struct bt_conn *conn, void *user_data);
	 void (*end)((struct bt_conn *conn, void *user_data);
}, 

int bt_conn_evt_cb_register(const struct bt_conn *conn, struct bt_conn_evt_cb cb,
			       void *user_data,
			       uint32_t prepare_distance_us);

@rugeGerritsen rugeGerritsen force-pushed the enhanced-radio-notification branch 3 times, most recently from dd25a10 to fc161a1 Compare July 4, 2024 08:48
@rugeGerritsen rugeGerritsen force-pushed the enhanced-radio-notification branch 2 times, most recently from 8c72532 to 1c492c7 Compare July 9, 2024 17:29
@rugeGerritsen rugeGerritsen marked this pull request as ready for review July 9, 2024 17:29
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Jul 9, 2024

Test specification

CI/Jenkins/NRF

  • Integration Platforms

CI/Jenkins/integration

Test Module File based changes Manually selected West overwrite
desktop52_verification X
test-fw-nrfconnect-apps X
test-fw-nrfconnect-ble_samples X
test-fw-nrfconnect-fem X
test-fw-nrfconnect-rs X
test-sdk-find-my X
test_ble_commit X

Detailed information of selected test modules

Note: This message is automatically posted and updated by the CI

@b-gent b-gent requested a review from peknis July 10, 2024 10:47
doc/nrf/protocols/bt/ble/conn_evt_cb.rst Outdated Show resolved Hide resolved
doc/nrf/protocols/bt/ble/conn_evt_cb.rst Outdated Show resolved Hide resolved
doc/nrf/protocols/bt/ble/conn_evt_cb.rst Outdated Show resolved Hide resolved
doc/nrf/protocols/bt/ble/conn_evt_cb.rst Outdated Show resolved Hide resolved
doc/nrf/protocols/bt/ble/conn_evt_cb.rst Outdated Show resolved Hide resolved
samples/bluetooth/conn_evt_cb/README.rst Outdated Show resolved Hide resolved
samples/bluetooth/conn_evt_cb/README.rst Outdated Show resolved Hide resolved
samples/bluetooth/conn_evt_cb/README.rst Outdated Show resolved Hide resolved
samples/bluetooth/conn_evt_cb/README.rst Outdated Show resolved Hide resolved
samples/bluetooth/conn_evt_cb/README.rst Outdated Show resolved Hide resolved
subsys/bluetooth/host_extensions/conn_evt_cb.c Outdated Show resolved Hide resolved
subsys/bluetooth/host_extensions/conn_evt_cb.c Outdated Show resolved Hide resolved
doc/nrf/protocols/bt/ble/conn_evt_cb.rst Outdated Show resolved Hide resolved
doc/nrf/protocols/bt/ble/conn_evt_cb.rst Outdated Show resolved Hide resolved
doc/nrf/protocols/bt/ble/conn_evt_cb.rst Outdated Show resolved Hide resolved
subsys/bluetooth/host_extensions/conn_evt_cb.c Outdated Show resolved Hide resolved
subsys/bluetooth/host_extensions/conn_evt_cb.c Outdated Show resolved Hide resolved
subsys/bluetooth/host_extensions/conn_evt_cb.c Outdated Show resolved Hide resolved
subsys/bluetooth/host_extensions/conn_evt_cb.c Outdated Show resolved Hide resolved
subsys/bluetooth/host_extensions/conn_evt_cb.c Outdated Show resolved Hide resolved
- nrf52dk/nrf52832
- nrf52840dk/nrf52840
- nrf5340dk/nrf5340/cpunet
platform_allow: nrf52dk/nrf52832 nrf52840dk/nrf52840 nrf5340dk/nrf5340/cpunet
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does not work on 54?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not yet. Need to add EGU definitions here: https://github.com/zephyrproject-rtos/zephyr/blob/main/modules/hal_nordic/nrfx/Kconfig#L64. I believe we can do that separately to unblock other work

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be possible after zephyrproject-rtos/zephyr#75866 is in NCS

@rugeGerritsen rugeGerritsen force-pushed the enhanced-radio-notification branch 3 times, most recently from 4ecc58a to 0e71b11 Compare July 15, 2024 07:16
The device may wake up more often if, for example, it has data to send.
The central device assumes that the peripheral will wake up at every connection event.

The :c:member:`ble_radio_notification_conn_cb.prepare` callback will be called before every connection event for both the peripheral and central device.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we combine this with the previous paragraph? I think they logically belong together.

The Radio Notification callback feature replaces
the radio notification feature. It can be used to
synchronize data sampling with Bluetooth connection events.

The old feature had several weaknesses:
 - It was not possible to know which connection
   the notification belonged to.
 - The trigger also happened upon flash access.
 - The time from trigger until event start was fixed.
   That is, not configurable per use case.

The new feature is built on top of the connection event
trigger feature and is therefore able to known
which connection a given notification belongs to.

The feature is marked as experimental.
Currently this feature will cancels out peripheral latency
which we indent to change.

Signed-off-by: Rubin Gerritsen <[email protected]>
The sample demonstrates how the Radio Notification connection
callback feature can used to minimize latency between data
sampling and on air transmission.

Signed-off-by: Rubin Gerritsen <[email protected]>
@rugeGerritsen rugeGerritsen dismissed alwa-nordic’s stale review July 17, 2024 12:30

Dismissing Aleksanders' review. The changes pushed later were pairprogrammed with him

@rlubos rlubos merged commit c5cd043 into nrfconnect:main Jul 17, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. doc-required PR must not be merged without tech writer approval.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants